-
-
Notifications
You must be signed in to change notification settings - Fork 6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixed, If the last value is the max or min, the range will be wrong #2229
Conversation
the last entry is ignored. If the last value is the max or min, the range will be wrong
Another problem. |
Codecov Report
@@ Coverage Diff @@
## master #2229 +/- ##
=========================================
Coverage ? 18.44%
=========================================
Files ? 124
Lines ? 14619
Branches ? 0
=========================================
Hits ? 2696
Misses ? 11923
Partials ? 0
Continue to review full report at Codecov.
|
Is there any issue to track this PR? we have no context for this change. |
I didn't notice other issues about this.
Go through this case Case 2: Just one entry Case 3: Based on the case 2, Just one entry So the correct code to fix the first two problems I think it may be like this
|
I just checked the android version .
|
@xuan @danielgindi could you please verify this |
@danielgindi should be still busy.. Haven't seen him recently. As this is a foundermental change, we need to have @danielgindi to review. |
😒
I think it's easy to reproduce this bug, you can verify it. And it's very reasonable, also android code can support it.
发自我的 iPhone
… 在 2017年4月1日,上午10:34,Xuan ***@***.***> 写道:
@danielgindi should be still busy.. Haven't seen him recently. As this is a foundermental change, we need to have @danielgindi to review.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
@danielgindi please take a look at this PR |
@aelam I am taking this PR right now, some thoughts:
This is rounding I took a look at We have to check the boundary again. if closest != -1
{
let closestXValue = _values[closest].x
if rounding == .up
{
// If rounding up, and found x-value is lower than specified x, and we can go upper...
if closestXValue < xValue && closest < _values.count - 1
{
closest += 1
}
} that Then, in another condition: // Search by closest to y-value
if !yValue.isNaN
{
while closest > 0 && _values[closest - 1].x == closestXValue
{
closest -= 1
}
var closestYValue = _values[closest].y
var closestYIndex = closest
while true
{
closest += 1
if closest >= _values.count { break }
let value = _values[closest]
if value.x != closestXValue { break }
if abs(value.y - yValue) < abs(closestYValue - yValue)
{
closestYValue = yValue
closestYIndex = closest
}
}
closest = closestYIndex
}
} Here
I guess that's why it uses However, in
It passes Share your thoughts if you can come up any other potential risks. I think this is positive to merge. |
should be safe to merge then with the analysis above and CI tests. |
@danielgindi @petester42 are there any plans to release a new version with the latest changes? This is a useful bugfix 🙏 |
It's already merged, so you can easily build one, or take the change and make a patch |
Fixed, If the last value is the max or min, the range will be wrong
The last entry is ignored.
If the last value is the max or min, the range will be wrong